Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sinatra extension registration #337

Merged
merged 4 commits into from
Sep 30, 2020
Merged

Conversation

bensymonds
Copy link
Contributor

It seems like pliny is not configuring its Serialize extension correctly. The extensions guide explains how to extend the DSL (as Serialize is doing here with its serializer method) - using register.

As is stands this module is depending on internal behaviour of sinatra - that it uses include to pull in helpers - so it was always at risk of breaking. In sinatra 2.1 it finally broke - the internal behaviour changed to use prepend.

#336 is a simple fix for this - switching the module to assume it is prepended rather than included - but there is no guarantee this behaviour won't change and break things again. Unfortunately the problem with this change is it requires a change in applications using pliny:

- helpers Pliny::Helpers::Serialize
+ register Pliny::Helpers::Serialize

I would probably just take the hit and go with this change, so we're protected from changes in future. Given pliny is still on a 0.x.y version it's "allowed" (according to SemVer) to break stuff. I doubt the number of applications using pliny is large, so we're not generating a large amount of effort downstream because of this.

`pry-byebug` is in the gemspec so we expect this file to exist sometimes.
This allows us to indent the text properly without the whitespace making it into the resulting string.
Looking at the sinatra extensions guide[1] it looks like you're meant to use `set` to set this kind of class-level state.

The value has to be initialised to nil otherwise you get `NoMethodError: undefined method `serializer_class'`, I guess because the accessor method is only created when `set` is called.

[1] http://sinatrarb.com/extensions.html#setting-options-and-other-extension-setup
Following the sinatra extensions guide[1] it looks like this is how you're meant to do it.

Unfortunately this results in a breaking change in pliny because pliny apps will have to switch their endpoint classes to use `register` instead of `helper` (see the change in the template in this commit). But it fixes a compatibility issue between pliny and sinatra 2.1 where the latter has switched its internal behaviour, using `prepend` rather than `include` for helpers, meaning the `self.included` hook is no longer being called.

[1] http://sinatrarb.com/extensions.html#extending-the-dsl-class-context-with-sinatraregister
@bensymonds bensymonds force-pushed the proper-extension-registration branch from 6773229 to ffefcdc Compare September 21, 2020 17:42
@bensymonds bensymonds merged commit 3c5b0ce into master Sep 30, 2020
@bensymonds bensymonds deleted the proper-extension-registration branch September 30, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants